Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add image editor #1317

Merged
merged 3 commits into from
Aug 17, 2022
Merged

Add image editor #1317

merged 3 commits into from
Aug 17, 2022

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Aug 9, 2022 •
edited by szaimen
Loading

image

Design review and todo

Overall

  • Edit would be nicer directly in the top right, outside of 3-dot menu. (Needs change in component, but would also be generally nice to have more actions outside of that 3-dot menu like download and "Open sidebar")
  • Edit icon can be simple pencil ("edit") from Material Icons
  • Background currently white, a bit jarring on the switch, better to use dark/black background so it’s similar to the Viewer background (but opaque). Not influenced by theming, always dark, like Viewer
    See Allow to manually specify a theme to apply to a section of an app server#33564
  • Default tab: Adjust. Default tool: Crop (always first thing in both lists)
  • Current primary color is purple from the library, would be nice if this can come from theming
  • Loading spinner is non-standard, we should use ours instead
  • Possibly remove ".jpeg" option in save dialog as simple ".jpg" is already there
  • When any dropdown is open, there’s a scrollbar on the right side of screen → Leads to interface moving around
  • Is the list of tools on the left accessible?
    • Ideally use our proper text color: color-main-text
    • Our text-size: 15px (navigation sizes need to be increased a bit)
  • Border-radius
    • Tools on left: border-radius-large
    • Tool details on bottom: border-radius
  • Would be nice if keyboard shortcuts work:
    • Ctrl+S for saving
    • Ctrl+Z for undo

Actions

  • Header bar of Viewer is not needed, can be overlapped
  • Reset button would be better with text "Reset" instead of icon, as it’s a very destructive action
    • When pressing reset, dialog with "Continue" action, action label would be better "Reset changes".
    • Also color would be better red than orange -> Not possible
  • Pressing close X button when changes are present should bring up an "Unsaved changes" dialog
    • Same for pressing Escape
  • Buttons like undo/redo, zoom in/out, and close could be tertiary icon-only buttons
  • "Side by side" better wording "View original"

Adjust

  • "Original" dropdown has large height which leads to layout issues

Resize

  • Lock button would look better icon-only

Watermark

  • Should be hidden as it’s the same as Draw (via Text and Image)

For later

  • Also are you sure that using a different name or file extension will always work? What about cases where you are only allowed to edit the file but not save in the target directory?
  • Is the list of tools on the left accessible?
  • Edit would be nicer directly in the top right, outside of 3-dot menu. (Needs change in component, but would also be generally nice to have more actions outside of that 3-dot menu like download and "Open sidebar")
  • Use dark theme
  • Disable when mobile
  • on a publlic page does the auto-generated path look a bit strange and the dropdown is missing some spacing on the left?

@skjnldsv skjnldsv added enhancement New feature or request 2. developing Work in progress labels Aug 9, 2022
@skjnldsv skjnldsv self-assigned this Aug 9, 2022
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 16, 2022
@skjnldsv skjnldsv requested review from a team, PVince81, artonge and Pytal and removed request for a team August 16, 2022 14:52
@skjnldsv skjnldsv added this to the Nextcloud 25 milestone Aug 16, 2022
@skjnldsv

This comment was marked as resolved.

Sign in to view
@skjnldsv
Add image editor
8623af4 
Signed-off-by: John Molakvoæ <[email protected]>
@szaimen

This comment was marked as resolved.

Sign in to view
@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 16, 2022 •
edited
Loading

Nice!
I guess I will fix it in a followup then.
I'm not even sure it's usable on mobile, Need to check with the UX team.

Some other issues to fix after:

  • The viewer doesn't refresh the saved image
  • The preview also doesn't seems to update

EDIT: Image saving is fixed

@skjnldsv
Fix editor image saving
34f2a29 
Signed-off-by: John Molakvoæ <[email protected]>
@szaimen

This comment was marked as resolved.

Sign in to view
@skjnldsv
Copy link
Member Author

  • Also are you sure that using a different name or file extension will always work? What about cases where you are only allowed to edit the file but not save in the target directory?

Good call! I need to have a look at that option too.

  • text is too dark?

Will fix small design in this PR.
Big other changes will be split for easier reviews :)

@szaimen szaimen mentioned this pull request Aug 16, 2022
Merged
@skjnldsv
Copy link
Member Author

Fixed
image

@skjnldsv skjnldsv requested a review from szaimen August 17, 2022 08:12
@skjnldsv
Editor design fixes
9e42436 
Signed-off-by: John Molakvoæ <[email protected]>
@szaimen
Copy link
Contributor

szaimen commented Aug 17, 2022

Two small things remaning:

  • on a publlic page does the auto-generated path look a bit strange and the dropdown is missing some spacing on the left?
    image

szaimen
szaimen approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in my testing when the follow-ups get addressed.
But did not really review the code.

@skjnldsv skjnldsv merged commit 2577cff into master Aug 17, 2022
@skjnldsv skjnldsv deleted the feat/editor branch August 17, 2022 10:20
This was referenced Aug 17, 2022
Merged
Merged
Closed
Merged
@szaimen
Copy link
Contributor

szaimen commented Aug 21, 2022

Do we want to add the remaining points to an overview ticket for easier discoverability? (Searching for this PR is not as easy imho)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szaimen szaimen szaimen approved these changes

@PVince81 PVince81 Awaiting requested review from PVince81

@artonge artonge Awaiting requested review from artonge

@Pytal Pytal Awaiting requested review from Pytal

Assignees

@skjnldsv skjnldsv

Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
2 participants
@skjnldsv @szaimen